-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[material-ui][Autocomplete] Fix more React 18.3 key spread warnings in demos #42639
Conversation
Netlify deploy previewhttps://deploy-preview-42639--material-ui.netlify.app/ Bundle size report |
It looks like I can't add a label on the PR as is required by test, and the GitHub editor doesn't apply prettier settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wbt Can you provide a live reproduction first of the error in CodeSandbox or StackBlitz?
It looks like that requires setting up a special billing account, getting an API key, and possibly breaking pseudonymity in open-source contributions more broadly just to re-demonstrate an issue that has several existing reports already. The existing example code does not need to be modified to demonstrate the issue; it appears that using it as-is in a project with React 18.3+ and your own API key would likely suffice to show it, assuming that NONE of the other issue reports appear credible. For quick setup close to the context where I saw it, I recommend running |
The demo in https://mui.com/material-ui/react-autocomplete/#globally-customized-options also has the same problem with the |
I saw other Autocomplete demos were updated in #42691. Maybe we need to update this one and this one as well? I'll leave this PR to be reviewed by @aarongarciah and @DiegoAndai since they have more context. |
We'll hold this PR until #42689 is merged so we get the correct types. Note that React 18.3.0 started to throw warnings when spreading This error, and the one in GloballyCustomizedOptions.tsx mentioned by @m4theushw, were harder to catch than the ones fixed in #42691 because these warnings only happen at runtime and the Google Maps demo didn't throw the warning until there are results, which is not the case if you don't have an API key 😅 I see that there are more warnings that we didn't catch because they are fired when the autocomplete is open and @wbt when a demo is modified (the .tsx file), you need to run |
35a7a27
to
facd8fb
Compare
Just pushed a commit that should solve all React warnings related to spreading key in Autocomplete demos. The I recommend reviewing this PR with the "Hide whitespace" option enabled. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aarongarciah I hope you don't mind the review. I was just scrolling through and found this.
ecf5a54
to
18520bf
Compare
I just rebased to get the latest typing fixes from #42689 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✨
…n demos (#42639) Signed-off-by: wbt <[email protected]> Co-authored-by: Aarón García Hervás <[email protected]>
…n demos (mui#42639) Signed-off-by: wbt <[email protected]> Co-authored-by: Aarón García Hervás <[email protected]>
Note: the PR has expanded to solve all errors in all Autocomplete demos, not only the Google Maps demo as the original PR description explained.
When using the demo code for autocomplete with Google Maps, there is an error thrown which seems to be discussed in other issues with respect to other broken examples, but I don't see a solution in place yet.
This addresses the Google Maps example only. There are probably other examples that should be updated too, but getting this merged seems like a positive step forward even if there are other positive steps forward that can follow.
Examples of issues closed as incomplete without a solution that seem to be reporting a similar error, especially with NextJS and React 18.3+:
This is still open:
The issue appears to have been partially addressed in
but this particular issue is still in the latest, so here's a step to fix it.
This borrows from #39833 (comment) in place of a slightly less elegant earlier draft.